Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid overwriting label when re-initialising Deployment object #145

Merged
merged 1 commit into from
Oct 8, 2019
Merged

Conversation

mikery
Copy link

@mikery mikery commented Oct 6, 2019

When I run the first example using the latest version on pip (0.4.2), the Nginx test fails with:

>       assert len(pods) == 3, 'nginx should deploy with three replicas'
E       AssertionError: nginx should deploy with three replicas
E       assert 0 == 3
E        +  where 0 = len([])

Some debugging showed that _add_kubetest_labels() is called twice: once by the applymanifests decorator, and once during deployments.get_deployments(). This results in the Deployment and Pods having differed UIDs, which causes get_pods() to return an empty list.

This patch fixes the problem by first checking to see if the deployment already has a UID label in its metadata. If so it is retrieved from the Deployment's label, instead of overwritten by a new UID.

There are already some PRs relating to this issue (e.g. #142), but label-based matching seems like a nice solution to the problem and disabling it entirely might lead to unpredictable results in more advanced multi-deployment test cases.

I lack sufficient familiarity with the codebase to know if this is the correct fix, but it is enough to get the example tests working as expected. I only tested deployments but statefulsets/daemonsets share the same code so presumably have the same issue.

@marcoceppi
Copy link

Hello, thank you for submitting a patch to kubetest! I've performed an initial scan of the changes and don't see any nits in formatting that might hold up a review. As such I've assigned @edaniszewski to take a look at this for a technical review

Copy link
Contributor

@edaniszewski edaniszewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a lot of sense! Thanks for debugging and contributing your solution (:

@edaniszewski edaniszewski merged commit 84d5a82 into vapor-ware:master Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants